chore: switch to module ESNext + moduleResolution bundler#2095
chore: switch to module ESNext + moduleResolution bundler#2095mattzcarey wants to merge 5 commits into
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Internal-only build configuration change. Consumers are not affected:
they continue to import from the built `.mjs`/`.d.mts` files declared
in each package's `exports` map.
What changed:
- `common/tsconfig/tsconfig.json`: `module: NodeNext` → `module: ESNext`,
`moduleResolution: NodeNext` → `moduleResolution: bundler`.
- `examples/{client,server}-quickstart/tsconfig.json`: same flip
(they extend a different base and overrode the resolution).
- Strip `.js` extensions from every relative TypeScript import across
packages/, examples/, scripts/, test/.
- Update CLAUDE.md to reflect the new import convention.
Why:
- Removes the long-standing footgun of having to write `from './foo.js'`
in `.ts` source files. Bundler resolution treats the path as a module
reference and lets the tooling resolve it.
- Aligns with what the bundler (`tsdown`), vitest, and downstream
consumers' bundlers actually do at runtime.
Verification: `pnpm typecheck:all`, `pnpm lint:all`, `pnpm test:all`,
`pnpm build:all` all pass.
- Make examples/{client,server}-quickstart extend @modelcontextprotocol/tsconfig
instead of carrying standalone compilerOptions. Drop the inline target/lib/
module/moduleResolution/strict/etc. duplicates; only outDir, rootDir,
declaration overrides, and workspace paths remain.
- Align docs/{client,server}-quickstart.md tsconfig snippets to ESNext/bundler
to match what the example projects now compile under.
- Adjust examples/client-quickstart/src/index.ts for the stricter
noUncheckedIndexedAccess inherited from the shared base.
- Strip leftover .js suffixes from vi.mock / vi.importActual string specifiers
in packages/client/test/client/middleware.test.ts.
The shared @modelcontextprotocol/tsconfig sets types: [node, vitest/globals], which the quickstarts inherit even though neither declares vitest as a devDependency — tsc only finds vitest types via the hoisted workspace root node_modules. Override types: [node] on each quickstart compilerOptions to break the accidental coupling.
…cli.ts
- Update docs/client-quickstart.md and docs/server-quickstart.md tsconfig
blocks to target: ESNext / lib: [ESNext], matching the shared base the
example tsconfigs now inherit from (was ES2023/ES2022, drifted after
consolidation onto @modelcontextprotocol/tsconfig).
- Delete scripts/cli.ts and the dangling server/client npm scripts that
referenced it across packages/{client,core,server}, packages/middleware/node,
test/{integration,helpers,conformance}, and examples/{client,server,shared}.
The file's imports point at ../src/* paths that ceased to exist post-monorepo;
no tsconfig builds it and no working npm script can reach it.
…kage The packages/codemod/ package (added in #1950) was written before the bundler resolution flip and used the old .js-extension import convention. This brings it in line with the rest of the repo. Backtick string fixtures that simulate user code (which intentionally still uses .js) are left alone.
mattzcarey
force-pushed
the
chore/bundler-tsconfig
branch
from
6307e1e to
d7b1c47
Compare
| - **Naming**: PascalCase for classes/types, camelCase for functions/variables | ||
| - **Files**: Lowercase with hyphens, test files with `.test.ts` suffix | ||
| - **Imports**: ES module style, include `.js` extension, group imports logically | ||
| - **Imports**: ES module style, no `.js` extension on relative imports (project uses `moduleResolution: bundler`), group imports logically |
There was a problem hiding this comment.
🟡 The rationale comment at common/eslint-config/eslint.config.mjs:39 ("Let the TS resolver handle NodeNext-style imports like "./foo.js"") now contradicts the import convention this PR establishes — CLAUDE.md was updated to say "no .js extension on relative imports (project uses moduleResolution: bundler)". The extensions resolver setting itself is still useful; only the comment is stale and should be updated or removed so future contributors aren't told the project still writes ./foo.js imports.
Extended reasoning...
What the bug is
This PR flips the project's module resolution from NodeNext to bundler and strips .js extensions from every relative TypeScript import across packages/, examples/, scripts/, and test/. It also updates CLAUDE.md line 39 to document the new convention: "ES module style, no .js extension on relative imports (project uses moduleResolution: bundler)". However, common/eslint-config/eslint.config.mjs was not touched, and line 39 there still reads:
// Let the TS resolver handle NodeNext-style imports like "./foo.js"
extensions: ['.js', '.jsx', '.ts', '.tsx', '.d.ts'],After this PR there are no NodeNext-style ./foo.js relative imports left in the repo (the PR's stated goal was to remove them all), so the comment now describes a convention the project explicitly migrated away from.
How it manifests
A contributor reading the eslint config — for example, while debugging an import-resolution lint error — would conclude from this comment that the project still writes relative imports with a .js extension and that the resolver setting exists to support that. That conclusion is the opposite of what CLAUDE.md now says and what every .ts file in the repo now does. Two pieces of in-repo documentation now actively contradict each other.
Why nothing catches it
The comment is plain prose inside a .mjs config file. pnpm lint:all and pnpm typecheck:all only verify code behavior, not whether comments accurately describe the surrounding configuration. There is no sync:snippets-style guard for config-file comments.
Step-by-step proof
- Before this PR,
CLAUDE.mdline 39 said: "ES module style, include.jsextension" — consistent with the eslint comment. - This PR changes
CLAUDE.mdline 39 to: "ES module style, no.jsextension on relative imports (project usesmoduleResolution: bundler)". - This PR also changes
common/tsconfig/tsconfig.jsonfrommodule: NodeNext/moduleResolution: NodeNexttomodule: ESNext/moduleResolution: bundler. common/eslint-config/eslint.config.mjs:39is unchanged:// Let the TS resolver handle NodeNext-style imports like "./foo.js".- Result: the eslint config's documented rationale references a NodeNext convention the project no longer uses, directly contradicting the freshly-updated
CLAUDE.md.
Why the extensions setting itself is fine
The extensions array tells eslint-import-resolver-typescript which file extensions to try when resolving an import specifier to a source file on disk. Even with extensionless imports under moduleResolution: bundler, the resolver still needs this list to map ./foo to foo.ts / foo.tsx / etc. So the setting should stay; only the reason given for it is stale.
Impact and fix
No functional impact — purely documentation drift. But the PR explicitly migrates the project's import convention and updates CLAUDE.md to match; leaving a contradicting comment in a sibling config file is exactly the kind of partial migration that confuses the next contributor. Update the comment to something like:
// Tell the TS resolver which file extensions to probe when resolving extensionless
// relative imports (project uses moduleResolution: bundler).
extensions: ['.js', '.jsx', '.ts', '.tsx', '.d.ts'],or simply remove the comment, since the setting is fairly self-explanatory.
| "lint:fix": "eslint src/ --fix && prettier --ignore-path ../../.prettierignore --write .", | ||
| "check": "pnpm run typecheck && pnpm run lint", | ||
| "test": "vitest run", | ||
| "test:watch": "vitest", | ||
| "server": "tsx watch --clear-screen=false scripts/cli.ts server", | ||
| "client": "tsx scripts/cli.ts client" | ||
| "test:watch": "vitest" | ||
| }, |
There was a problem hiding this comment.
🟡 After this PR removes the server/client npm scripts (the only tsx invocations in these packages), the tsx devDependency is now unused in five manifests: packages/core/package.json:84, packages/client/package.json:103, packages/server/package.json:101, packages/middleware/node/package.json:77, and examples/shared/package.json:54. Consider dropping "tsx": "catalog:devTools" from each — test/conformance still legitimately uses it via npx tsx ./src/everythingClient.ts, so leave that one.
Extended reasoning...
What was left behind
This PR removes the dangling server and client npm scripts from packages/core, packages/client, packages/server, packages/middleware/node, and examples/shared (e.g. "server": "tsx watch --clear-screen=false scripts/cli.ts server" and "client": "tsx scripts/cli.ts client"), and deletes the orphaned scripts/cli.ts they pointed at. Those two scripts were the only thing in each of these five packages that invoked tsx. After their removal, the remaining scripts are typecheck (tsgo), lint/lint:fix (eslint, prettier), check (pnpm run), test/test:watch (vitest), and build/build:watch (tsdown, in middleware/node) — none of them run tsx. Yet each manifest still lists "tsx": "catalog:devTools" in its devDependencies.
Why nothing else flags it
There is no CI step that detects unreferenced devDependencies, and pnpm install will happily keep installing tsx into each package's node_modules whether or not anything uses it. Per-package vitest.config.js, eslint.config.mjs, and tsdown.config.ts files in these five packages also do not depend on tsx, so there is no indirect consumer either.
Step-by-step proof
- Pre-PR,
packages/core/package.jsonhad"server": "tsx watch --clear-screen=false scripts/cli.ts server"and"client": "tsx scripts/cli.ts client", plus"tsx": "catalog:devTools"indevDependencies. Same pattern in the other four packages. - This PR's diff deletes the
serverandclientscript entries from all fivepackage.jsonfiles (and deletesscripts/cli.ts). - After the PR, grepping each of the five manifests for
tsxreturns only thedevDependenciesentry — atpackages/core/package.json:84,packages/client/package.json:103,packages/server/package.json:101,packages/middleware/node/package.json:77, andexamples/shared/package.json:54. No remaining script invokes it. test/conformance/package.jsonstill has scripts like"test:conformance:client": "conformance client --command 'npx tsx ./src/everythingClient.ts' ..."so itstsxdevDependency is still load-bearing and should stay.
Impact
No functional or runtime impact — this is pure dependency hygiene. The cost is a slightly heavier node_modules install for each of these five packages and a stale entry that will mislead the next person reading the manifests into thinking tsx is still in use there.
Fix
Delete the "tsx": "catalog:devTools" line from devDependencies in:
packages/core/package.json(line 84)packages/client/package.json(line 103)packages/server/package.json(line 101)packages/middleware/node/package.json(line 77)examples/shared/package.json(line 54)
Then run pnpm install to refresh pnpm-lock.yaml. Leave test/conformance's tsx dependency alone.
1 participant
Summary
common/tsconfig/tsconfig.jsonfrommodule: NodeNext/moduleResolution: NodeNexttomodule: ESNext/moduleResolution: bundler.Node16overrides inexamples/client-quickstart/tsconfig.jsonandexamples/server-quickstart/tsconfig.json..jsextensions from every relative TypeScript import acrosspackages/,examples/,scripts/, andtest/.CLAUDE.mdto reflect the new import convention.Why
Under
moduleResolution: NodeNext, every relative TS import has to be written with a.jsextension (import x from './foo.js') — a long-standing footgun for new contributors and a frequent source of confusion when scripts/codemods generate imports.moduleResolution: bundlertreats the path as a module reference, matching whattsdown,vitest, and downstream consumers' bundlers already do at runtime.This is an internal-only build configuration change. Consumers are not affected: they continue to import from the built
.mjs/.d.mtsfiles declared in each package'sexportsmap, which still carry the.jsextensions Node's NodeNext resolver requires at runtime.Test plan
pnpm typecheck:allpassespnpm lint:allpasses (includingsync:snippets --check)pnpm test:allpassespnpm build:allpasses